Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#504): npm publish fails / npm publish <folder> publishes from cwd folder #531

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Badisi
Copy link

@Badisi Badisi commented Dec 3, 2022

Fixes #504

@Badisi
Copy link
Author

Badisi commented Dec 17, 2022

Anyone ? please

@greglbd
Copy link

greglbd commented May 6, 2023

This is a great solution! I just ran and tested it and even tried your suggested patch.

Copy link

@greglbd greglbd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once the conflict and test report is resolved this looks like a winner.

I've tested this in 2 monorepos using lerna and this solves my problems with pkgRoot not being honoured.

['publish', basePath, '--userconfig', npmrc, '--tag', distTag, '--registry', registry],
{cwd, env, preferLocal: true}
['publish', '--userconfig', npmrc, '--tag', distTag, '--registry', registry],
{cwd: basePath, env, preferLocal: true}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd like to point out that when we specify tarballDir it doesn't suffer the same problem and it's because it uses exactly this solution in the prepare.js file see: https://github.com/semantic-release/npm/blob/master/lib/prepare.js#L14-L22

@Badisi
Copy link
Author

Badisi commented May 25, 2023

@greglbd, thanks for testing that up, I appreciate 😉

@travi, I don't like having to ping someone, but since it's been 6 months already, could we have at least a word from you on this issue please ? Thanks 😊

@fadler82
Copy link

fadler82 commented Dec 7, 2023

any progress here? I have the same issue and would like to get rid of this patch

@mario-jerkovic
Copy link

@gr2m Is there any chance to merge this?

@stevefrench39
Copy link

stevefrench39 commented Aug 29, 2024

Reporting live from August 28, 2024

@jedwards1211
Copy link

I put this fix into my own @jcoreio/semantic-release-npm and have been using it successfully in several pnpm monorepos for quite awhile now.
@travi @gr2m @babblebey is there any opposition to this? I'd like to help get it merged

@travi
Copy link
Member

travi commented Sep 3, 2024

could someone help me the use case where this is necessary vs setting main/exports and files appropriately?

@travi
Copy link
Member

travi commented Sep 3, 2024

to move this forward, we would need the conflicts resolved and make sure that this scenario is covered by a test

@jedwards1211
Copy link

jedwards1211 commented Sep 3, 2024

@travi in my case I am passing the pkgRoot: 'dist' option to @semantic-release/npm (or rather, my fork that includes this fix) because I build everything into the dist folder (including a transformed package.json) and publish from there.

I need to confirm whether I had issues with pkgRoot in single package repos or just in monorepos, but regardless, as @greglbd pointed out, the prepare call is using cwd: basePath, whereas the publish call is using cwd. The difference seems like an accident, and I doubt there is any justification for it?

@travi
Copy link
Member

travi commented Sep 3, 2024

I am passing the pkgRoot: 'dist' option to @semantic-release/npm (or rather, my fork that includes this fix) because I build everything into the dist folder (including a transformed package.json) and publish from there

i understand that folks tend to do this, but i've never understood why. can you help me grasp the why of that decision?

i bundle into either a dist or lib in most of my packages as well, but just include that directory in the path of what is defined as main or exports. then, i basically just include that folder in the files property so that all other files are excluded when publishing to the registry.

i'm open to fixing pkgRoot either way, but i'm wondering if we should add documentation directing folks to the alternative that at least seems simpler to me that maybe some folks dont understand is an option

@jedwards1211
Copy link

jedwards1211 commented Sep 3, 2024

Yeah there are several reasons.

Back when I started doing it support for export maps wasn't very widespread, and I wanted import {foo} from 'my-pkg/foo' to work without export maps, which is only possible if foo.js lives in the package root. Otherwise the subpath imports would have to look like my-pkg/dist/foo. Now that export maps are widely supported, you would think this is no longer necessary; but, unfortunately the default TypeScript compiler options, which ignore export maps, are still widespread among library consumers afaik. So even if the export map works fine with bundlers/runtimes, I would still have to tell library consumers to change their TS config.

The alternative to this is to build src/foo.ts -> ./foo.js and publish from the root dir. This can also work, but you have to be careful what you name files and subdirectories inside src/, and it makes cleaning up the build output more fraught.

Aside from that, I figured I might as well strip dev-only fields from the package.json I publish, and I like the peace of mind that comes with publishing from isolated build output, not having to worry about .npmignore or "files": [...] etc.

@travi
Copy link
Member

travi commented Sep 3, 2024

those are helpful. thanks for taking the time to explain 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm publish fails / npm publish <folder> publishes from cwd folder
8 participants